Move secret name or ID prefix resolving from client to daemon #29218

yongtang · 2016-12-07T19:14:54Z

- What I did

This fix is a follow up for comment:
#28896 (comment)

Currently secret name or ID prefix resolving is done at the client side, which means different behavior of API and CMD.

/cc @thaJeztah @ehazlett @allencloud

- How I did it
This fix is a follow up for comment:
#28896 (comment)

Currently secret name or ID prefix resolving is done at the client side, which means different behavior of API and CMD.

- How to verify it

All existing tests should pass.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

This fix is related to #28896, #28884 and may be related to #29125.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

ehazlett · 2016-12-07T20:00:36Z

nice -- i had started something to work on 29125 but this is better.

LGTM (nam)

thaJeztah · 2016-12-08T13:54:50Z

daemon/cluster/secrets.go

 )

+func getSecretByNameOrIDPrefix(ctx context.Context, state *nodeState, nameOrIDPrefix string) (*swarmapi.Secret, error) {
+	r, err := state.controlClient.ListSecrets(ctx, &swarmapi.ListSecretsRequest{


I think this is an optimization, correct? Now that we run this on the daemon, is this still needed?

I don't fully know how the ListSecrets filters work; is it possible that that filter hides a secret, that would otherwise be successfully matched here?

Thanks @thaJeztah. The filters in the ListSecrets will operates as OR so only one call is needed. This is different from some other SwarmKit API such as listing tasks ListTasks, which does not operate on OR.

I think it might make sense to optimize some other SwarmKit APIs such as ListTasks, if ListSecrets already operates on OR. Will open a PR in SwarmKit to seek additional discussions.

thaJeztah · 2016-12-08T13:56:08Z

daemon/cluster/secrets.go

+	}
+
+	// attempt to lookup secret by full ID
+	for _, s := range r.Secrets {


Instead of looping; does the secret-store already have aGetSecret() that always matches on ID? If it exists; would it be better to use that as the primary way to get the secret?

Ah yes swarm api could get the Secret from ID directly 👍 . Let me update the PR.

@thaJeztah Now full ID match is done through swarm kit's GetSecret directly. ListSecrets will do the Full Name and Partial ID match, only be called if GetSecret returns nothing.

cpuguy83 · 2016-12-21T01:21:57Z

daemon/cluster/secrets.go

+	// attempt to lookup secret by full name
+	for _, s := range r.Secrets {
+		if s.Spec.Annotations.Name == nameOrIDPrefix {
+			return s, nil


Shouldn't we error out if len(r.Secrets) > 0 before this for loop?

Reason being if we get more than one, then it's an ambiguous result.
Also cleans up some of the stuff below.

Thanks @cpuguy83 for the review. In swarmkit, the filter of secrets is slightly different from other filters (like services) in that it is operated as OR:
https://github.com/docker/docker/blob/v1.13.0-rc4/vendor/github.com/docker/swarmkit/manager/controlapi/secret.go#L121-L132

In the above case, we passes both Names and IDPrefixes so the result could be multiple like one for name and one for IDPrefix. This is considered as non-ambiguous as we have a full name match.

cpuguy83

LGTM

yongtang · 2016-12-22T16:31:37Z

/cc @thaJeztah

aaronlehmann · 2017-01-14T00:09:55Z

Code LGTM

@yongtang: This has API impact, so api/swagger.yaml needs to be updated.

@thaJeztah: Are we comfortable making an API change like this to endpoints that have been added in 1.13?

thaJeztah · 2017-01-14T01:46:52Z

Are we comfortable making an API change like this to endpoints that have been added in 1.13?

Ideally, this would've been part of 1.13, but I don't think that's still realistic, so;

I think this change is fully backward compatible with the current 1.13 API (i.e., providing a full ID still returns the same information, correct?
We could gate the "fuzzy" search (name, partial ID) for the 1.13 (API 1.25) version, or just keep it as "undocumented" enhancement. would there be situations where this could have an unwanted side effects for 1.13 clients?

For the documentation; I'm not sure how to define a change for a specific version in the swagger.yml. @dnephin @bfirsh perhaps you can assist with that?

Otherwise; good to go 😄 - I'm changing the milestone to 1.14, unless we see reasons/options to include this in 1.13 (@vieux?)

thaJeztah · 2017-01-14T01:48:48Z

Actually; if this goes to 1.14, the CLI should still do "client side" lookup when talking to a 1.13 daemon. 😞

bfirsh · 2017-01-14T08:54:29Z

@thaJeztah Each API version has its own swagger.yml. For this change, there is no issue because there is no swagger.yml representation of 1.24 or lower.

At some point we'll have to make a copy of swagger.yml for 1.25 docs when 1.26 becomes the current docs (if that hasn't happened already?). The original intention was to keep these old versions in the docs repository, because in theory they should not be updated again and any changes would just be bug fixes. For simplicity of contribution, however, perhaps it makes sense for them to live in this repo somewhere.

dnephin

LGTM

dnephin · 2017-01-16T17:01:20Z

Actually; if this goes to 1.14, the CLI should still do "client side" lookup when talking to a 1.13 daemon.

I don't think that's necessary. We can just make that an error or warning, and inform the user that it won't work as expected (only works with exact IDs in that case).

thaJeztah · 2017-01-16T22:02:09Z

@dnephin the point is that basic functionality of docker secret won't work if we don't fall back to the old behavior; using the client from this PR, against docker 1.13

Client:
 Version:      1.14.0-dev
 API version:  1.25 (downgraded from 1.26)
 Go version:   go1.7.3
 Git commit:   a097d6c
 Built:        Mon Jan 16 21:30:03 2017
 OS/Arch:      linux/amd64

Server:
 Version:      1.13.0-rc6
 API version:  1.25 (minimum version 1.12)
 Go version:   go1.7.3
 Git commit:   2f2d055
 Built:        Wed Jan 11 21:47:55 2017
 OS/Arch:      linux/amd64
 Experimental: false

Create a secret

$ echo "foo" | docker secret create foo
veltdcwwvv5qdewcxs85n8tj2

$ docker secret ls

ID                          NAME                CREATED             UPDATED
veltdcwwvv5qdewcxs85n8tj2   foo                 4 minutes ago       4 minutes ago

then try to either inspect or rm it;

$ docker secret inspect foo
[]
Error: no such secret: foo
$ docker secret rm foo
Error response from daemon: rpc error: code = 5 desc = could not find secret foo

dnephin · 2017-01-16T22:17:05Z

@thaJeztah I understand that. So we can add a warning about it in the 1.14 client. When the 1.14 downgrades it can warn about having to use ids.

thaJeztah · 2017-01-16T23:07:45Z

I'd like to get input from @vieux on that; I don't think it's good to have support for talking to older daemons, but at the same time break that functionality. I think we'll only be supporting going one or two versions back, in which case, keeping the fallback mechanism should still be an option. (but that's just my 0.02c)

LK4D4 · 2017-01-27T18:31:05Z

@yongtang needs rebase, sorry :)

This fix is a follow up for comment: moby#28896 (comment) Currently secret name or ID prefix resolving is done at the client side, which means different behavior of API and CMD. This fix moves the resolving from client to daemon, with exactly the same rule: - Full ID - Full Name - Partial ID (prefix) All existing tests should pass. This fix is related to #288896, moby#28884 and may be related to moby#29125. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2017-01-27T18:48:49Z

Thanks @LK4D4 the PR has been rebased.

LK4D4 · 2017-01-27T21:30:39Z

This already has 2 LGTMs

GordonTheTurtle added the status/0-triage label Dec 7, 2016

ehazlett mentioned this pull request Dec 7, 2016

secrets: secret rm ignores non-existing secrets #29125

Closed

ehazlett added the version/1.13 label Dec 7, 2016

thaJeztah reviewed Dec 8, 2016

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Dec 8, 2016

thaJeztah added this to the 1.13.0 milestone Dec 8, 2016

yongtang force-pushed the 28884-secret-inspect-follow-up branch 2 times, most recently from 441c8e8 to a097d6c Compare December 8, 2016 15:34

cpuguy83 reviewed Dec 21, 2016

View reviewed changes

cpuguy83 approved these changes Dec 21, 2016

View reviewed changes

thaJeztah modified the milestones: 1.14.0, 1.13.0 Jan 14, 2017

thaJeztah added impact/api impact/changelog labels Jan 14, 2017

dnephin approved these changes Jan 16, 2017

View reviewed changes

LK4D4 removed the version/1.13 label Jan 27, 2017

yongtang force-pushed the 28884-secret-inspect-follow-up branch from a097d6c to fa358a8 Compare January 27, 2017 18:45

LK4D4 merged commit 3c32e17 into moby:master Jan 27, 2017

yongtang deleted the 28884-secret-inspect-follow-up branch January 27, 2017 21:44

This was referenced Jan 28, 2017

[WIP]make remove multiple secret run normally #29138

Closed

remove cli/command/secrets/utils.go #30534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move secret name or ID prefix resolving from client to daemon #29218

Move secret name or ID prefix resolving from client to daemon #29218

yongtang commented Dec 7, 2016

ehazlett commented Dec 7, 2016

thaJeztah Dec 8, 2016

yongtang Dec 8, 2016

thaJeztah Dec 8, 2016

yongtang Dec 8, 2016

yongtang Dec 8, 2016

cpuguy83 Dec 21, 2016

cpuguy83 Dec 21, 2016

yongtang Dec 21, 2016

cpuguy83 left a comment

yongtang commented Dec 22, 2016

aaronlehmann commented Jan 14, 2017

thaJeztah commented Jan 14, 2017

thaJeztah commented Jan 14, 2017

bfirsh commented Jan 14, 2017 •

edited

dnephin left a comment

dnephin commented Jan 16, 2017

thaJeztah commented Jan 16, 2017

dnephin commented Jan 16, 2017

thaJeztah commented Jan 16, 2017

LK4D4 commented Jan 27, 2017

yongtang commented Jan 27, 2017

LK4D4 commented Jan 27, 2017

Move secret name or ID prefix resolving from client to daemon #29218

Move secret name or ID prefix resolving from client to daemon #29218

Conversation

yongtang commented Dec 7, 2016

ehazlett commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

yongtang commented Dec 22, 2016

aaronlehmann commented Jan 14, 2017

thaJeztah commented Jan 14, 2017

thaJeztah commented Jan 14, 2017

bfirsh commented Jan 14, 2017 • edited

dnephin left a comment

Choose a reason for hiding this comment

dnephin commented Jan 16, 2017

thaJeztah commented Jan 16, 2017

dnephin commented Jan 16, 2017

thaJeztah commented Jan 16, 2017

LK4D4 commented Jan 27, 2017

yongtang commented Jan 27, 2017

LK4D4 commented Jan 27, 2017

bfirsh commented Jan 14, 2017 •

edited